Conversation
This module defines the TokenAssociation class, which provides an immutable representation of token-account associations as reported by the Hedera network. It includes methods for converting to and from protobuf representations. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Updated the TransactionRecord class to include new attributes and improve documentation. Enhanced type hints and refined the __repr__ method for better readability. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hiero_sdk_python/transaction/transaction_record.py (2)
246-258:⚠️ Potential issue | 🔴 Critical
oneof entropynot handled —prng_numberandprng_byteswill both receive proto default values instead ofNone.Proto fields 19 and 20 form
oneof entropy { bytes prng_bytes; int32 prng_number; }. Reading both directly (lines 257–258) means the unset branch returns its scalar default (0forprng_number,b""forprng_bytes) rather thanNone. This also violates guideline 3d which requiresHasFieldorWhichOneofto detect the populated branch.Additionally,
transactionHash(line 248) is abytesfield typedbytes | Nonebut is not normalized — it will beb""when unset instead ofNone.Proposed fix
return cls( transaction_id=tx_id, - transaction_hash=proto.transactionHash, + transaction_hash=proto.transactionHash or None, transaction_memo=proto.memo, transaction_fee=proto.transactionFee, receipt=TransactionReceipt._from_proto(proto.receipt, tx_id), token_transfers=token_transfers, nft_transfers=nft_transfers, transfers=transfers, new_pending_airdrops=new_pending_airdrops, call_result=call_result, - prng_number=proto.prng_number, - prng_bytes=proto.prng_bytes, + prng_number=( + proto.prng_number + if proto.HasField("prng_number") + else None + ), + prng_bytes=( + proto.prng_bytes or None + if proto.HasField("prng_bytes") + else None + ),As per coding guidelines, "
_from_protousesproto.HasField(...)(not attribute access) to detect which branch of aoneofis populated" and "If the SDK models the field asbytes | None, verify that_from_protousesproto.<field> or Noneto normalize empty bytes toNone."
365-382:⚠️ Potential issue | 🔴 Critical
oneof entropyguard missing in_to_proto, and passingNonefor scalar proto fields will raiseTypeError.Two issues in the constructor block:
Missing
oneof entropyguard: There is a mutual-exclusivity check forcall_result/contract_create_result(thebodyoneof) at lines 366–370, but no equivalent guard forprng_number/prng_bytes(theentropyoneof). Both may be non-Nonedue to the_from_protobug above or direct user construction.
Nonepassed to scalar proto fields: Iftransaction_hash,transaction_memo,transaction_fee,prng_number, orprng_bytesareNone, the protobuf constructor will raiseTypeError(scalars don't acceptNone). These fields were changed toX | Nonetypes but the serialization wasn't updated to match.Proposed fix
# Defensive guard for protobuf oneof if self.call_result is not None and self.contract_create_result is not None: raise ValueError( "call_result and contract_create_result are mutually exclusive " "(protobuf oneof) — only one may be set at a time" ) + + if self.prng_number is not None and self.prng_bytes is not None: + raise ValueError( + "prng_number and prng_bytes are mutually exclusive " + "(protobuf oneof entropy) — only one may be set at a time" + ) - record_proto = transaction_record_pb2.TransactionRecord( - transactionHash=self.transaction_hash, - memo=self.transaction_memo, - transactionFee=self.transaction_fee, - receipt=self.receipt._to_proto() if self.receipt else None, - contractCallResult=( - self.call_result._to_proto() if self.call_result else None - ), - prng_number=self.prng_number, - prng_bytes=self.prng_bytes, - ) + kwargs = {} + if self.transaction_hash is not None: + kwargs["transactionHash"] = self.transaction_hash + if self.transaction_memo is not None: + kwargs["memo"] = self.transaction_memo + if self.transaction_fee is not None: + kwargs["transactionFee"] = self.transaction_fee + if self.receipt is not None: + kwargs["receipt"] = self.receipt._to_proto() + if self.call_result is not None: + kwargs["contractCallResult"] = self.call_result._to_proto() + if self.prng_number is not None: + kwargs["prng_number"] = self.prng_number + if self.prng_bytes is not None: + kwargs["prng_bytes"] = self.prng_bytes + + record_proto = transaction_record_pb2.TransactionRecord(**kwargs)As per coding guidelines, "Verify that
_to_protodoes NOT set more than one field from the sameoneofblock simultaneously. If it does, aValueErrorguard MUST be present before setting those fields" and "if_to_protosets a bytes field, verify it guards against settingNone(which would cause a protobuf type error)."
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.py
| from dataclasses import dataclass | ||
| from typing import Optional |
There was a problem hiding this comment.
Use X | None instead of Optional[X] for consistency with codebase style.
The rest of the codebase (including the companion transaction_record.py in this PR) uses the X | None union syntax. New code should follow the same convention.
Proposed fix
from dataclasses import dataclass
-from typing import Optional
from hiero_sdk_python.account.account_id import AccountId- token_id: Optional[TokenId] = None
+ token_id: TokenId | None = None
"""The identifier of the token being associated."""
- account_id: Optional[AccountId] = None
+ account_id: AccountId | None = None
"""The identifier of the account receiving the association."""As per coding guidelines, "The codebase uses X | None union syntax (Python 3.10+). Flag any use of Optional[X] from typing in newly added or modified code, as it is inconsistent with the established style."
Also applies to: 50-50, 53-53
| from hiero_sdk_python.account.account_id import AccountId | ||
| from hiero_sdk_python.tokens.token_id import TokenId | ||
| from hiero_sdk_python.hapi.services.basic_types_pb2 import TokenAssociation as TokenAssociationProto |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the modules exist and verify their paths
echo "=== Checking AccountId file ==="
find . -name "account_id.py" -type f 2>/dev/null | head -5
echo "=== Checking TokenId file ==="
find . -name "token_id.py" -type f 2>/dev/null | head -5
echo "=== Checking basic_types_pb2 ==="
find . -name "basic_types_pb2.py" -type f 2>/dev/null | head -5
echo "=== Verifying AccountId class exists ==="
rg "class AccountId" --type=py -A 2
echo "=== Verifying TokenId class exists ==="
rg "class TokenId" --type=py -A 2
echo "=== Checking for TokenAssociation in basic_types_pb2 ==="
rg "class TokenAssociation" src/hiero_sdk_python/hapi/ --type=pyRepository: MonaaEid/hiero-sdk-python
Length of output: 915
🏁 Script executed:
#!/bin/bash
# Search for protobuf files in the codebase
echo "=== Finding all *_pb2.py files ==="
find . -name "*_pb2.py" -type f 2>/dev/null | head -20
echo "=== Searching for basic_types_pb2 anywhere ==="
find . -name "*basic_types*" -type f 2>/dev/null
echo "=== Listing src/hiero_sdk_python structure ==="
ls -la src/hiero_sdk_python/ 2>/dev/null | head -20
echo "=== Checking if hapi or similar protobuf module exists ==="
find src/hiero_sdk_python -type d -maxdepth 1 2>/dev/nullRepository: MonaaEid/hiero-sdk-python
Length of output: 1878
🏁 Script executed:
#!/bin/bash
# Read the actual token_association.py file
echo "=== Contents of token_association.py ==="
cat src/hiero_sdk_python/tokens/token_association.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 3276
Remove or correct the import of non-existent hiero_sdk_python.hapi.services.basic_types_pb2.
The hiero_sdk_python.hapi module does not exist in the codebase. The AccountId and TokenId imports are valid, but the line importing TokenAssociation as TokenAssociationProto from hiero_sdk_python.hapi.services.basic_types_pb2 references a module path that is not present. This will cause an ImportError at runtime.
Verify whether:
- Protobuf files should be generated at build time (and are currently missing)
- The protobuf dependency is configured correctly
- An alternative import path should be used
| from hiero_sdk_python.schedule.schedule_id import ScheduleId | ||
| from hiero_sdk_python.timestamp import Timestamp | ||
| from hiero_sdk_python.tokens.assessed_custom_fee import AssessedCustomFee | ||
| from hiero_sdk_python.tokens.token_airdrop_pending_record import PendingAirdropRecord | ||
| from hiero_sdk_python.tokens.token_association import TokenAssociation |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking ScheduleId module ==="
fd 'schedule_id.py' --type f
echo ""
echo "=== Checking Timestamp module ==="
fd 'timestamp.py' --type f -x grep -n '_from_protobuf\|_to_protobuf\|_from_proto\|_to_proto' {}
echo ""
echo "=== Checking AssessedCustomFee module ==="
fd 'assessed_custom_fee.py' --type f
echo ""
echo "=== Checking for ScheduleId._from_proto ==="
rg -n '_from_proto|_to_proto' --type=py -g '*schedule_id.py'
echo ""
echo "=== Checking for AssessedCustomFee._from_proto / _to_proto ==="
rg -n '_from_proto|_to_proto' --type=py -g '*assessed_custom_fee.py'
echo ""
echo "=== Checking PendingAirdropRecord module ==="
fd 'token_airdrop_pending_record.py' --type f
echo ""
echo "=== Checking TokenAssociation module ==="
fd 'token_association.py' --type fRepository: MonaaEid/hiero-sdk-python
Length of output: 1883
🏁 Script executed:
#!/bin/bash
echo "=== Checking transaction_record.py for Timestamp usage ==="
sed -n '195,210p' src/hiero_sdk_python/transaction/transaction_record.py | cat -n
echo ""
echo "=== Lines 405-420 ==="
sed -n '405,420p' src/hiero_sdk_python/transaction/transaction_record.py | cat -n
echo ""
echo "=== Check all Timestamp method calls in transaction_record.py ==="
rg -n 'Timestamp\._from_protobuf|Timestamp\._to_protobuf' src/hiero_sdk_python/transaction/transaction_record.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 1980
🏁 Script executed:
#!/bin/bash
echo "=== Check all _from_protobuf usage in codebase ==="
rg -l '_from_protobuf' --type=py | head -20
echo ""
echo "=== Check all _to_protobuf usage in codebase ==="
rg -l '_to_protobuf' --type=py | head -20
echo ""
echo "=== Count _from_proto vs _from_protobuf ==="
echo "_from_proto count:"
rg -c '_from_proto' --type=py | wc -l
echo "_from_protobuf count:"
rg -c '_from_protobuf' --type=py | wc -l
echo ""
echo "=== List all files using _from_protobuf ==="
rg -l '_from_protobuf' --type=pyRepository: MonaaEid/hiero-sdk-python
Length of output: 2853
🏁 Script executed:
#!/bin/bash
echo "=== Classes defining _from_protobuf ==="
rg 'def _from_protobuf' --type=py
echo ""
echo "=== Classes defining _to_protobuf ==="
rg 'def _to_protobuf' --type=py
echo ""
echo "=== Check custom_fee.py methods ==="
rg 'def _from_proto|def _to_proto' --type=py src/hiero_sdk_python/tokens/custom_fee.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 938
Verify that newly imported modules exist and expose the expected APIs; also confirm Timestamp method naming inconsistency.
All modules exist and are correctly imported:
ScheduleId✓ (uses_from_proto/_to_proto)AssessedCustomFee✓ (uses_from_proto/_to_proto)PendingAirdropRecord✓TokenAssociation✓
However, the concern about Timestamp's method naming is valid: Timestamp defines _from_protobuf and _to_protobuf (lines 107, 97 in src/hiero_sdk_python/timestamp.py), whereas ScheduleId, AssessedCustomFee, and all other SDK message wrapper classes use the standard _from_proto / _to_proto naming convention. This inconsistency is evident in transaction_record.py at lines 201, 207, 413, 416 where Timestamp methods are called. The codebase is actively standardizing on _from_proto / _to_proto (custom_fee.py was recently refactored with this change), so Timestamp should be updated to match.
| TokenId, list[TokenNftTransfer] | ||
| ] = field(default_factory=lambda: defaultdict(list)) | ||
| transfers: defaultdict[AccountId, int] = field(default_factory=lambda: defaultdict(int)) | ||
| new_pending_airdrops: list[PendingAirdropRecord] = field(default_factory=list) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Looking for existing TransactionRecord tests ==="
rg -l 'TransactionRecord' --type=py -g '*test*'
echo ""
echo "=== Looking for test references to new fields ==="
rg -n 'consensus_timestamp|assessed_custom_fees|automatic_token_associations|paid_staking_rewards|contract_create_result|ethereum_hash|evm_address' --type=py -g '*test*'Repository: MonaaEid/hiero-sdk-python
Length of output: 41207
🏁 Script executed:
#!/bin/bash
# Check the transaction_record_test.py file size and content
echo "=== Checking transaction_record_test.py ==="
wc -l tests/unit/transaction_record_test.py
echo ""
echo "=== Looking for round-trip or _from_proto/_to_proto tests ==="
rg -n '_from_proto|_to_proto|round.?trip' tests/unit/transaction_record_test.py | head -20
echo ""
echo "=== Checking for tests of specific TransactionRecord fields ==="
rg -n 'consensus_timestamp|assessed_custom_fees|paid_staking_rewards|contract_create_result|contract_call_result|ethereum_hash|pending_airdrops' tests/unit/transaction_record_test.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 3331
🏁 Script executed:
#!/bin/bash
# Read the TransactionRecord class to see all fields
echo "=== TransactionRecord class structure ==="
sed -n '1,150p' src/hiero_sdk_python/transaction/transaction_record.py | cat -n
echo ""
echo "=== Continue reading TransactionRecord fields ==="
sed -n '50,100p' src/hiero_sdk_python/transaction/transaction_record.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 13629
🏁 Script executed:
#!/bin/bash
echo "=== Looking at _from_proto method ==="
rg -n '_from_proto' src/hiero_sdk_python/transaction/transaction_record.py -A 150 | head -200Repository: MonaaEid/hiero-sdk-python
Length of output: 8710
🏁 Script executed:
#!/bin/bash
echo "=== Looking at _to_proto method (continuation) ==="
sed -n '340,450p' src/hiero_sdk_python/transaction/transaction_record.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 5667
Test coverage exists but is incomplete for new TransactionRecord fields.
Tests for TransactionRecord do exist (tests/unit/transaction_record_test.py), and some new fields are tested: new_pending_airdrops has both _from_proto and round-trip tests, and prng_number is covered.
However, the following fields added in this PR lack dedicated test coverage: consensus_timestamp, parent_consensus_timestamp, schedule_ref, assessed_custom_fees, automatic_token_associations, paid_staking_rewards, contract_create_result, ethereum_hash, evm_address, and alias. These should have dedicated _from_proto, _to_proto, and round-trip tests, particularly given the oneof constraint on call_result and contract_create_result and the bytes normalization required for ethereum_hash and evm_address.
1 participant
No description provided.